stdlib: Add a _FilterProtocol type to logging's Filterer.addFilter() filter argument. #11018
Conversation
This comment has been minimized.
This comment has been minimized.
Akuli
left a comment
Akuli
left a comment
There was a problem hiding this comment.
Looks good! A couple nits.
|
Also, I’m not sure about your merge habits (do you squash the PR or not) so I’m just going to keep amending to the original commit so there’s just one commit to merge. Keeps the history clean. |
jenstroeger
force-pushed
the
add-filter-protocol
branch
from
77d7efd to
3525fcb
Compare
We always squash-merge at typeshed :) see our CONTRIBUTING guidelines: https://github.com/python/typeshed/blob/main/CONTRIBUTING.md#submitting-changes |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Akuli
left a comment
Akuli
left a comment
There was a problem hiding this comment.
This is turning out trickier than expected :)
stdlib/logging/__init__.pyi
Outdated
| _ExcInfoType: TypeAlias = None | bool | _SysExcInfoType | BaseException | ||
| _ArgsType: TypeAlias = tuple[object, ...] | Mapping[str, object] | ||
| _FilterType: TypeAlias = Filter | Callable[[LogRecord], bool] | ||
| _FilterType: TypeAlias = _SupportsFilter | Callable[[LogRecord], bool] |
There was a problem hiding this comment.
- Turns out we should keep
Filterhere, based on the mypy_primer output. Sphinx uses# type: ignorewhen they define a custom filter with a weirdfilter()method. IncludingFilterhere means that they don't get errors when adding the custom filter, only when defining it. Getting errors in only one place is better IMO. - On 3.12+, we should probably support the
LogRecordreturn type here too.
| _FilterType: TypeAlias = _SupportsFilter | Callable[[LogRecord], bool] | |
| if sys.version_info >= (3, 12): | |
| _FilterType: TypeAlias = Filter | _SupportsFilter | Callable[[LogRecord], bool | LogRecord] | |
| else: | |
| _FilterType: TypeAlias = Filter | _SupportsFilter | Callable[[LogRecord], bool] |
This comment has been minimized.
This comment has been minimized.
|
I was thinking that by making |
|
Pytest and whoever else needs to annotate "just some filter" can use |
Akuli
left a comment
Akuli
left a comment
There was a problem hiding this comment.
Thanks for working on this! One more nit.
stdlib/logging/__init__.pyi
Outdated
|
|
||
| if sys.version_info >= (3, 12): | ||
| class _SupportsFilter(Protocol): | ||
| def filter(self, record: LogRecord) -> bool | LogRecord: ... |
There was a problem hiding this comment.
Let's make the argument positional-only in the protocol, so if your class names it something else than Record, it will still match the protocol:
| def filter(self, record: LogRecord) -> bool | LogRecord: ... | |
| def filter(self, __record: LogRecord) -> bool | LogRecord: ... |
There was a problem hiding this comment.
I made the change in commit fe68d1a, but I’m still a little unclear: is the dunder __record a convention that the name is now “flexible”?
It doesn’t make the record arg (not capitalized Record) positional-only, though, that would require a / as well, right?
There was a problem hiding this comment.
Type-checkers special-case __dunder-named arguments and treat them as positional-only. This is a bit surprising, and we will soon be able to switch to the new / syntax.
There was a problem hiding this comment.
This is documented in PEP 484: https://peps.python.org/pep-0484/#positional-only-arguments
(PEP 484 predates PEP 570, which was only implemented in Python 3.8 -- typeshed still supports Python 3.7.)
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
3 participants
Closes #5700
As discussed in the related issue, starting with comment #5700 (comment).